-
Notifications
You must be signed in to change notification settings - Fork 1k
update opentelemetry sdk to 1.58.0 #15817
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update opentelemetry sdk to 1.58.0 #15817
Conversation
7e2bdd1 to
4968924
Compare
|
@trask please have a look |
| * This class is internal and is hence not for public use. Its APIs are unstable and can change at | ||
| * any time. | ||
| */ | ||
| public final class ExtendedDeclarativeConfigProperties implements DeclarativeConfigProperties { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
| ExtendedOpenTelemetry otel = mock(ExtendedOpenTelemetry.class); | ||
| ConfigProvider configProvider = mock(ConfigProvider.class); | ||
| ExtendedOpenTelemetry otel = spy(ExtendedOpenTelemetry.class); | ||
| ConfigProvider configProvider = spy(ConfigProvider.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This enables the default method on both interfaces to be called
| .satisfiesExactlyInAnyOrder( | ||
| metric -> assertThat(metric).hasName("otel.sdk.span.started"), | ||
| metric -> assertThat(metric).hasName("otel.sdk.span.live"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why doesn't this affect other metric tests? do they not use satisfies "exactly"? if not, we could do that here for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - let me do that in a follow up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just checked: all other metric tests also use satisfiesExactlyInAnyOrder - they just happen to not encounter this metric, because no span is created
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for checking. I think ok for now. if it becomes a problem in the future we can consider loosening the condition or automatically filtering out the sdk metrics
No description provided.